Added tests for parsley message definitions by yashjain128 · Pull Request #47 · waterloo-rocketry/parsley

yashjain128 · 2025-10-29T22:43:54Z

For waterloo-rocketry/2025-software-issues#60

Created 46 tests, 2 tests per packet format specified in https://docs.waterloorocketry.com/avionics/rocketcan/packet-format.html

Deleted the old tests. The code currently passes all the tests.

Removed references to broken test_utils.py which will need to be removed from other test files

This change is

Summary by CodeRabbit

Tests
- Replaced an older CAN message test suite with a new, focused byte-level parser test suite covering board status, reset/config flows, actuator/altimeter commands and states, sensors (temp/IMU/mag/baro/analog), GPS fields, state estimation, and LED/time handling.
Chores
- Added test coverage tooling to dev dependencies.

ChrisYx511

Few comments:

I feel like your tests are redundant. Why are there 2 tests for every message definition when a lot of the messages are testing for the exact same thing. Each test should test something different and they shouldn't just be numbered.
Watch unnecessary comments. AI loves adding them but your code should explain itself. You should not need a comment to understand your code except when defining stuff values and binary constants (like arbitrary binary values from the docs). Even then you can use good variable naming to avoid it.

You're on the right track! Please review your changes and re-request review!

tests/test_message_definitions.py

ChrisYx511 · 2025-10-30T23:05:09Z

Also please name your PR something more descriptive and include screenshots of coverage and tests passing.

coderabbitai · 2025-11-04T21:37:57Z

Walkthrough

Removed an old CAN-message test module, added a new comprehensive parser test module for CAN message definitions, and updated dev dependencies (added pytest-cov).

Changes

Cohort / File(s)	Summary
Deleted CAN message tests `tests/test_can_messages.py`	Entire test module removed: all fixtures (`bit_str2`, `bit_str3`), TestCANMessage class and its many pytest cases for CAN parsing were deleted.
Added/expanded message-definition tests `tests/test_message_definitions.py`	New comprehensive test suite using `parsley` parsing utilities: constructs timestamped `BitString` payloads, exercises field defs (`Enum`, `Numeric`, `ASCII`, `Floating`, `Bitfield`), and asserts parsed values across board/config/actuator/altimeter/sensors/GPS/state-est/LED messages.
Dev dependency update `pyproject.toml`	Added `pytest-cov>=7.0.0` to the dev dependencies group.

Sequence Diagram(s)

sequenceDiagram
  participant Test as pytest test
  participant Builder as BitString builder
  participant Parser as parsley.parse_fields
  participant Specs as Field definitions (MESSAGES / Enum/Numeric/Bitfield/Floating)
  participant Output as parsed result

  Note over Test,Builder: Arrange — build payload with TIMESTAMP_2
  Test->>Builder: assemble bytes / timestamp
  Builder->>Parser: pass BitString + message spec
  Parser->>Specs: evaluate each field (enum/bitfield/float/ASCII)
  Specs-->>Parser: field values
  Parser-->>Output: aggregated parsed dict
  Output-->>Test: assertions (expected values)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Inspect deleted tests/test_can_messages.py to ensure no remaining references elsewhere.
Verify new tests/test_message_definitions.py byte layouts, endianness, and field definitions align with current message specs.
Confirm pyproject.toml dev dependency addition follows project grouping and lockfile expectations.

Possibly related PRs

Fixing test_parsley.py #45 — Related test-suite updates and migrations to the parsley parsing API and message definitions.

Suggested reviewers

JasonBrave
ChrisYx511

Poem

🐰
I hopped through bytes at break of dawn,
Swapped timestamps, fields — then leapt upon
Bitfields, floats, and parsers true;
Tests now dance in parsley dew,
Carrots for CI — a tasty run! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Added tests for parsley message definitions' accurately describes the main change: comprehensive test suite additions for CAN message definitions using parsley.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch y29jain/add-tests-can-messages-#60

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

yashjain128 · 2025-11-04T21:39:49Z

Hey @ChrisYx511 I have made the changes.

ChrisYx511 · 2025-11-05T00:21:33Z

Hey @ChrisYx511 I have made the changes.

@yashjain128 Please use the "Request Rereview" button so I see it quicker next time.

ChrisYx511

close! good work! please address some of the systemic issues with the tests you've written

ChrisYx511 · 2025-11-05T00:23:49Z

tests/test_message_definitions.py

+        assert res["curr_state"] == "ACT_STATE_ON"
+        assert res["cmd_state"] == "ACT_STATE_OFF"
+
+    def test2_actuator_status(self, bit_str2):


how is this test different? your name doesn't tell me

ChrisYx511 · 2025-11-05T00:29:34Z

tests/test_message_definitions.py

+from pytest import approx
+from parsley.bitstring import BitString
+from parsley.fields import ASCII, Enum, Numeric, Floating, Bitfield
+from parsley.message_definitions import TIMESTAMP_2, MESSAGES


I don't see you using the actual message definitions? isn't that the whole point of this test? Why are you building your own fields?

ChrisYx511

Also nit: don't include your issue number like that in the title it's pretty useless. Include a link in your description. Especially since the issue is not even on the same repository.

…s.py

yashjain128 · 2025-11-05T01:24:57Z

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/test_message_definitions.py (1)

24-208: Consider adding edge case and error handling tests.

The current tests provide excellent happy-path coverage. To make the test suite more robust, consider adding tests for:

Boundary values (min/max for Numeric fields, empty ASCII strings)

Invalid enum values that should raise exceptions

Out-of-range values that exceed field bit lengths

Malformed payloads (insufficient data for field definitions)

This would help catch potential issues with field validation and error handling in the parsley library.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca918ba and 2603c96.

⛔ Files ignored due to path filters (1)

uv.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

pyproject.toml (1 hunks)
tests/test_can_messages.py (0 hunks)
tests/test_message_definitions.py (1 hunks)

💤 Files with no reviewable changes (1)

tests/test_can_messages.py

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_message_definitions.py (3)

src/parsley/bitstring.py (2)

BitString (1-49)

push (30-41)

src/parsley/fields.py (5)

ASCII (38-62)

Enum (64-106)

Numeric (108-144)

Floating (146-179)

Bitfield (199-232)

src/parsley/parsley.py (1)

parse_fields (12-27)

🔇 Additional comments (4)

tests/test_message_definitions.py (3)

11-22: LGTM! Well-structured test fixture.

The test class and fixture setup is clean and efficient. The fixture properly initializes a BitString with the TIMESTAMP_2 field, and the docstring clearly explains the testing scope.

138-146: Clarify the pressure value discrepancy.

There's a mismatch between the comment and the assertion:

Comment states: "pressure 101325 (0x018B6D)"

Assertion expects: pressure == 101229

The difference is 96. Please verify whether this is due to:

A scaling factor or unit conversion in the Numeric field definition

An error in the comment

Rounding or precision handling

If this is due to scaling, consider updating the comment to reflect the actual expected decoded value to avoid confusion.

209-211: Clarify the intent of this test.

This test parses an empty fields list and asserts an empty result. If the LEDS_ON message intentionally has no body fields beyond the timestamp (already in bit_str2), this test is valid but could benefit from a comment explaining that.

However, if LEDS_ON should have LED state fields, this test appears incomplete.

Please confirm whether LEDS_ON is expected to have only a timestamp field, or if additional fields should be tested here.

pyproject.toml (1)

39-39: No issues found — pytest-cov 7.0.0 exists and is the latest version.

The latest pytest-cov release is 7.0.0, uploaded to PyPI on September 9, 2025. The version constraint in pyproject.toml is valid and will not cause dependency resolution failures.

ChrisYx511 · 2025-11-13T19:34:43Z

@yashjain128 get another review and merge this!!

ShreyShingala

LGTM!

JasonBrave · 2025-11-17T05:08:19Z

pls rebase with latest main branch

yashjain128 requested review from ChrisYx511 and ShreyShingala October 29, 2025 22:43

yashjain128 requested a review from JasonBrave as a code owner October 29, 2025 22:43

ChrisYx511 requested changes Oct 30, 2025

View reviewed changes

tests/test_message_definitions.py Show resolved Hide resolved

yashjain128 changed the title ~~Y29jain/add tests can messages #60~~ Added 24 tests for test_can_messages.py (issue #60) Nov 4, 2025

yashjain128 changed the title ~~Added 24 tests for test_can_messages.py (issue #60)~~ Added 24 tests for parsley message definitions (issue #60) Nov 4, 2025

ChrisYx511 requested changes Nov 5, 2025

View reviewed changes

ChrisYx511 reviewed Nov 5, 2025

View reviewed changes

yashjain128 changed the title ~~Added 24 tests for parsley message definitions (issue #60)~~ Added tests for parsley message definitions Nov 5, 2025

yashjain128 requested a review from ChrisYx511 November 5, 2025 01:01

Yash Jain added 6 commits November 4, 2025 20:19

Added 2 tests for each message packet definition.

f7b00f4

Removed print statements

8c78179

removed import test_utils

ef07330

Fixed test_can_messages. Removed half the tests and uneccessary comments

6de9efe

Rename test_can_messages.py to test_message_definitions.py

a5ec228

Replaced hard coded definitions with MESSAGES from message_definition…

c16f98c

…s.py

Added coverage to dev dependencies

2603c96

yashjain128 force-pushed the y29jain/add-tests-can-messages-#60 branch from ca918ba to 2603c96 Compare November 5, 2025 01:31

ChrisYx511 approved these changes Nov 5, 2025

View reviewed changes

coderabbitai bot reviewed Nov 5, 2025

View reviewed changes

yashjain128 mentioned this pull request Nov 5, 2025

[TASK] Creating test for test_can_messages.py waterloo-rocketry/2025-software-issues#60

Open

ShreyShingala removed their request for review November 12, 2025 05:06

ShreyShingala approved these changes Nov 15, 2025

View reviewed changes

Merge branch 'main' into y29jain/add-tests-can-messages-#60

3ac37d5

yashjain128 merged commit 815e430 into main Nov 20, 2025
3 of 4 checks passed

yashjain128 deleted the y29jain/add-tests-can-messages-#60 branch November 20, 2025 21:36

coderabbitai bot mentioned this pull request Nov 26, 2025

Refactored Parse Function #50

Merged

Conversation

yashjain128 commented Oct 29, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

ChrisYx511 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisYx511 commented Oct 30, 2025

Uh oh!

coderabbitai bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

yashjain128 commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisYx511 commented Nov 5, 2025

Uh oh!

ChrisYx511 left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisYx511 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

ChrisYx511 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

ChrisYx511 left a comment

Choose a reason for hiding this comment

Uh oh!

yashjain128 commented Nov 5, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisYx511 commented Nov 13, 2025

Uh oh!

ShreyShingala left a comment

Choose a reason for hiding this comment

Uh oh!

JasonBrave commented Nov 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yashjain128 commented Oct 29, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 4, 2025 •

edited

Loading

yashjain128 commented Nov 4, 2025 •

edited

Loading